feat: scoped accounts#161
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR migrates the interpreter balance representation from a map to a row-based ChangesBalances model, typed value semantics, and scoped builtin
Sequence Diagram(s)sequenceDiagram
participant Parser
participant Analyzer
participant Interpreter
participant CachedBalances
participant Store
Parser->>Analyzer: scoped(`@src`, "x")
Analyzer->>Analyzer: checkFnCall validates FnVarOriginScoped registered
Interpreter->>Interpreter: evaluate scoped(account, scope)
Interpreter->>Interpreter: expectAccount validates `@src`
Interpreter->>Interpreter: expectString extracts "x"
Interpreter->>Interpreter: validateScope checks ^[a-z0-9_]*$
Interpreter-->>Parser: AccountAddress{Name: "src", Scope: "x"}
Parser->>Interpreter: send via scoped source
Interpreter->>CachedBalances: fetchBalance(src/x, asset, color)
CachedBalances-->>Interpreter: *big.Int balance
Interpreter->>Store: query balances for src/x
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ascandone
force-pushed
the
feat/scoped-accounts
branch
from
3613cf9 to
bcba9a9
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/specs_format/index.go (1)
87-107:⚠️ Potential issue | 🟡 MinorCheck function has an implicit precondition: specs must be pre-validated, but this is not documented.
While the production code path in
runRawSpec(runner.go:160) does guardCheckwithvalidateSpecs, theCheckfunction is exported and lacks documentation of this precondition. Tests intentionally callCheckdirectly without validation—which is fine for unit testing—but any external caller or future code path that directly invokesCheckcould bypass the guard.Consider either:
- Adding a docstring to
Checkdocumenting the precondition thatvalidateSpecsmust be called first- Making
Checkinternal (unexported) to enforce the contract at the type level- Moving validation into
Checkitself to ensure it always holds🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/specs_format/index.go` around lines 87 - 107, The exported Check function has an implicit precondition that the specs parameter must be pre-validated by calling validateSpecs first, but this requirement is not documented. Add a docstring to the Check function that clearly documents this precondition, explaining that callers must ensure specs have been validated before invoking Check. This will make the contract explicit and prevent external callers or future code paths from accidentally bypassing the validation guard.
♻️ Duplicate comments (2)
internal/analysis/check.go (1)
127-131:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the scoped feature flag for
scoped()version gating.Line 130 still references
flags.ExperimentalGetAmountFunctionFeatureFlagforFnVarOriginScoped. This makes static analysis require the wrong flag forscoped()and can diverge from interpreter behavior. Switch it toflags.ExperimentalScopedFunction.Proposed fix
FnVarOriginScoped: VarOriginFnCallResolution{ Params: []string{TypeAccount, TypeString}, Return: TypeAccount, Docs: "returns the scoped version of that account. Empty string means no scope. Overwrites the previous scope", VersionConstraints: []VersionClause{ { Version: parser.NewVersionInterpreter(0, 0, 25), - FeatureFlag: flags.ExperimentalGetAmountFunctionFeatureFlag, + FeatureFlag: flags.ExperimentalScopedFunction, }, }, },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/analysis/check.go` around lines 127 - 131, The VersionClause for FnVarOriginScoped (the scoped() function) in the check.go file is using the incorrect feature flag. Replace flags.ExperimentalGetAmountFunctionFeatureFlag with flags.ExperimentalScopedFunction in the FeatureFlag field of the VersionClause that has Version 0.0.25. This ensures that static analysis uses the correct scoped feature flag for version gating and aligns with the interpreter behavior.internal/interpreter/interpreter.go (1)
184-184:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep canonical account validation free of the DSL
@sigil.Line 184 now accepts an optional leading
@. This allows raw variable values like@srcto validate as account addresses and propagate into postings/store-facing data, which breaks canonical account contract boundaries.Suggested fix
-var accountNameRegex = regexp.MustCompile("^@?" + accountSegmentRegex + "(:" + accountSegmentRegex + ")*(?:/[a-z_]+)?$") +var accountNameRegex = regexp.MustCompile("^" + accountSegmentRegex + "(:" + accountSegmentRegex + ")*(?:/[a-z_]+)?$")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/interpreter/interpreter.go` at line 184, The accountNameRegex variable at line 184 includes an optional leading @ character (^@?) which allows DSL-specific variable values like `@src` to be validated as canonical account addresses, violating the account contract boundary. Remove the @? optional matching from the beginning of the regex pattern so it only validates canonical account names without the DSL @ sigil prefix.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/interpreter/balances.go`:
- Around line 83-88: The CompareBalances function can incorrectly return true
for non-equal multisets when duplicate (account, asset, color) keys exist,
because CompareBalancesIncluding only resolves the first match of each key. Add
duplicate key validation in the CompareBalances function to check both input
Balances for duplicate keys and return false immediately if any duplicates are
found, ensuring that only well-formed (non-duplicate) Balances can pass the
equality check.
In `@internal/interpreter/evaluate_expr.go`:
- Around line 154-158: The code unconditionally calls colorExpr.GetRange() when
constructing the InvalidColor error return at line 157, but colorExpr can be nil
according to the evaluateOptExprAs contract. Add a nil check for colorExpr
before dereferencing it; if colorExpr is nil, either omit the Range field from
the InvalidColor struct or provide a zero/default Range value instead of calling
GetRange().
In `@internal/interpreter/function_exprs.go`:
- Around line 181-185: Remove the invalid pointer dereferences in the scoped()
function. The parameters scope and acc are value types (string type aliases),
not pointers, so they should not be dereferenced with the * operator. Remove the
* prefix from all occurrences: the *scope in the validateScope call, the *scope
in the InvalidScope struct literal, and both *acc and *scope in the appendScope
call.
In `@internal/interpreter/internal_balances.go`:
- Around line 71-82: In the Set method of InternalBalances, the code currently
stores caller-owned *big.Int pointers directly (at both the assignment on line
74 and the append on line 81), which allows external mutation of cached state
and permits nil pointers to enter the runtime balance state. Copy the amount
value instead of storing the pointer directly: create a new big.Int and use the
Set method to copy the value (e.g., new(big.Int).Set(amount)) at both locations
where Amount is assigned, and ensure nil amounts are handled appropriately by
normalizing them to zero or a safe default big.Int value.
In `@internal/interpreter/store.go`:
- Around line 47-52: Add nil checks before dereferencing row.Amount in the
BalanceRow initialization within the store materialization code. When
constructing BalanceRow objects and assigning Amount using
new(big.Int).Set(row.Amount), first check if row.Amount is nil to prevent panics
during query execution. Apply this same nil-checking pattern to all locations
where row.Amount is dereferenced through the Set method call, ensuring the code
gracefully handles null amount values.
In `@internal/mcp_impl/handlers.go`:
- Around line 69-72: The error handling for BindArguments is inconsistent with
other error handling in this handler. Currently, when BindArguments fails, the
code returns (nil, err) which propagates it as a server-level error. Refactor
this to wrap the error in mcp.NewToolResultError(...) and return
(mcp.NewToolResultError(...), nil) instead, matching the pattern used for other
errors in the same handler such as RequireString failures and interpreter
errors. This ensures all tool-level errors are handled consistently as
structured tool result errors rather than server-level errors.
---
Outside diff comments:
In `@internal/specs_format/index.go`:
- Around line 87-107: The exported Check function has an implicit precondition
that the specs parameter must be pre-validated by calling validateSpecs first,
but this requirement is not documented. Add a docstring to the Check function
that clearly documents this precondition, explaining that callers must ensure
specs have been validated before invoking Check. This will make the contract
explicit and prevent external callers or future code paths from accidentally
bypassing the validation guard.
---
Duplicate comments:
In `@internal/analysis/check.go`:
- Around line 127-131: The VersionClause for FnVarOriginScoped (the scoped()
function) in the check.go file is using the incorrect feature flag. Replace
flags.ExperimentalGetAmountFunctionFeatureFlag with
flags.ExperimentalScopedFunction in the FeatureFlag field of the VersionClause
that has Version 0.0.25. This ensures that static analysis uses the correct
scoped feature flag for version gating and aligns with the interpreter behavior.
In `@internal/interpreter/interpreter.go`:
- Line 184: The accountNameRegex variable at line 184 includes an optional
leading @ character (^@?) which allows DSL-specific variable values like `@src` to
be validated as canonical account addresses, violating the account contract
boundary. Remove the @? optional matching from the beginning of the regex
pattern so it only validates canonical account names without the DSL @ sigil
prefix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ce101ddb-95c1-4b5f-a5ba-b512764f07fe
⛔ Files ignored due to path filters (140)
inputs.schema.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/numscript-cookbook/experimental/mixed-source-prefer-single-source.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/numscript-cookbook/experimental/top-up-many.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/numscript-cookbook/experimental/top-up.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/numscript-cookbook/experimental/transfer-example.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/numscript-cookbook/mixed-source-dest-allotment.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/numscript-cookbook/send-max.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/numscript-cookbook/top-up-max.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/allocate-dont-take-too-much.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/allocation.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/ask-balance-twice.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/balance-not-found.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/balance-simple.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/balance.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/bigint-literal.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/capped-when-less-than-needed.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/capped-when-more-than-balance.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/cascading-sources.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/do-not-exceed-overdraft-on-send-all.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/do-not-exceed-overdraft-when-double-spending.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/do-not-exceed-overdraft.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/dynamic-allocation.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/empty-postings.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/account-interpolation/account-interp.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-colors/color-inorder-send-all.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-colors/color-inorder.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-colors/color-restrict-balance-when-missing-funds.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-colors/color-restrict-balance.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-colors/color-restriction-in-send-all.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-colors/color-send-overdrat.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-colors/color-send.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-colors/color-with-asset-precision.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-colors/empty-color.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-colors/no-double-spending-in-colored-send-all.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-colors/no-double-spending-in-colored-send.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-scaling/no-solution.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-scaling/scaling-all-allotment.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-scaling/scaling-allotment.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-scaling/scaling-kept.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-scaling/scaling-send-all.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-scaling/scaling-with-oneof.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-scaling/scaling.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/asset-scaling/update-swap-account-balance.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/get-amount-function/get-amount-function.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/get-asset-function/get-asset-function.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/mid-script-function-call/expr-in-var-origin.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/mid-script-function-call/midscript-balance-after-decrease.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/mid-script-function-call/midscript-balance.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/oneof/oneof-all-failing.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/oneof/oneof-in-send-all.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/oneof/oneof-in-source-send-first-branch.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/oneof/oneof-in-source.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/oneof/oneof-singleton.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/oneof/update-balances-with-oneof.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/overdraft-function/overdraft-function-use-case-remove-debt.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/overdraft-function/overdraft-function-when-negative.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/overdraft-function/overdraft-function-when-positive.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/overdraft-function/overdraft-function-when-zero.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/overdraft-function/reach-zero.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/experimental/scoped-function/simple.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/feature-flag-syntax.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/floating-perc.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/floating-perc2.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/inoder-destination.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/insufficient-funds.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/kept-in-send-all-inorder.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/kept-with-balance.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/many-kept-dest.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/many-max-dest.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/max-with-unbounded-overdraft.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/metadata.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/minus-infix-monetary.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/minus-infix-number.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/minus-prefix-number.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/neg-max-dest.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/negative-max-send-all.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/negative-max.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/nested-remaining-complex.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/ovedrafts-playground-example.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/overdraft-in-send-all-when-noop.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/overdraft-in-send-all.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/overdraft-not-enough-funds.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/overdraft-when-enough-funds.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/overdraft-when-negative-balance-in-send-all.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/overdraft-when-negative-balance.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/overdraft-when-negative-ovedraft-in-send-all.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/overdraft-when-not-enough-funds.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/remaining-kept-in-send-all-inorder.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/remaining-none-in-send-all.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/save/save-from-account__multi-postings.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/save/save-from-account__save-a-different-asset.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/save/save-from-account__save-all-negative-balance.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/save/save-from-account__save-all.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/save/save-from-account__save-causes-failure.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/save/save-from-account__save-more-than-balance.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/save/save-from-account__simple.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/save/save-from-account__with-asset-var.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/save/save-from-account__with-monetary-var.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/send-all-destinatio-allot-complex.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/send-all-destinatio-allot.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/send-all-many-max-in-dest.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/send-all-multi.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/send-all-variable.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/send-all-when-negative-with-overdraft.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/send-all-when-negative.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/send-all.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/send-allt-max-in-dest.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/send-allt-max-in-src.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/send-allt-max-when-no-amount.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/send-when-negative-balance.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/send-zero.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/send.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/source-allotment-invalid-amt.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/source-allotment.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/source-complex.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/source-overlapping.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/source.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/track-balances-send-all.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/track-balances.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/track-balances2.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/track-balances3.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/unbounded-overdraft-when-not-enough-funds.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/update-balances.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/use-balance-twice.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/use-different-assets-with-same-source-account.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/variable-asset.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/variable-balance__1.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/variable-balance__2.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/variable-balance__3.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/variable-balance__4.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/variable-balance__5.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/variables-json.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/variables.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/world-source.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/zero-postings-explicit-allotment.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/zero-postings-explicit-inorder.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/zero-postings.num.specs.jsonis excluded by!**/*.jsoninternal/parser/__snapshots__/parser_test.snapis excluded by!**/*.snap,!**/*.snapinternal/specs_format/__snapshots__/runner_test.snapis excluded by!**/*.snap,!**/*.snapspecs.schema.jsonis excluded by!**/*.json
📒 Files selected for processing (36)
internal/analysis/check.gointernal/cmd/run.gointernal/cmd/test_init.gointernal/cmd/test_init_test.gointernal/flags/flags.gointernal/interpreter/accounts_metadata.gointernal/interpreter/append_scope.gointernal/interpreter/append_scope_test.gointernal/interpreter/args_parser.gointernal/interpreter/args_parser_test.gointernal/interpreter/asset_scaling.gointernal/interpreter/balances.gointernal/interpreter/balances_test.gointernal/interpreter/batch_balances_query.gointernal/interpreter/evaluate_expr.gointernal/interpreter/function_exprs.gointernal/interpreter/function_statements.gointernal/interpreter/infix_overloads.gointernal/interpreter/internal_balances.gointernal/interpreter/internal_balances_test.gointernal/interpreter/interpreter.gointernal/interpreter/interpreter_error.gointernal/interpreter/interpreter_test.gointernal/interpreter/store.gointernal/interpreter/testdata/script-tests/experimental/scoped-function/simple.numinternal/interpreter/value.gointernal/mcp_impl/handlers.gointernal/parser/ast.gointernal/specs_format/index.gointernal/specs_format/parse_test.gointernal/specs_format/run_test.gointernal/specs_format/runner.gointernal/specs_format/runner_test.gointernal/utils/utils.gonumscript.gonumscript_test.go
💤 Files with no reviewable changes (2)
- internal/utils/utils.go
- internal/parser/ast.go
| isValidColor := colorRe.Match([]byte(string(color))) | ||
| if !isValidColor { | ||
| return nil, InvalidColor{ | ||
| return "", InvalidColor{ | ||
| Range: colorExpr.GetRange(), | ||
| Color: *color, | ||
| Color: string(color), |
There was a problem hiding this comment.
Guard nil colorExpr before using GetRange().
Line 157 dereferences colorExpr even though evaluateOptExprAs permits colorExpr == nil. If empty color is rejected, this can panic at runtime.
Proposed fix
func (s *programState) evaluateColor(colorExpr parser.ValueExpr) (String, InterpreterError) {
+ if colorExpr == nil {
+ return "", nil
+ }
+
color, err := evaluateOptExprAs(s, colorExpr, expectString)
if err != nil {
return "", err
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| isValidColor := colorRe.Match([]byte(string(color))) | |
| if !isValidColor { | |
| return nil, InvalidColor{ | |
| return "", InvalidColor{ | |
| Range: colorExpr.GetRange(), | |
| Color: *color, | |
| Color: string(color), | |
| func (s *programState) evaluateColor(colorExpr parser.ValueExpr) (String, InterpreterError) { | |
| if colorExpr == nil { | |
| return "", nil | |
| } | |
| color, err := evaluateOptExprAs(s, colorExpr, expectString) | |
| if err != nil { | |
| return "", err | |
| } | |
| // ... rest of function continues |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/interpreter/evaluate_expr.go` around lines 154 - 158, The code
unconditionally calls colorExpr.GetRange() when constructing the InvalidColor
error return at line 157, but colorExpr can be nil according to the
evaluateOptExprAs contract. Add a nil check for colorExpr before dereferencing
it; if colorExpr is nil, either omit the Range field from the InvalidColor
struct or provide a zero/default Range value instead of calling GetRange().
| err = request.BindArguments(&args) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Inconsistent error handling for BindArguments failure.
Other errors in this handler (e.g., RequireString at line 53, parse errors at line 62, interpreter errors at line 84) return (mcp.NewToolResultError(...), nil), treating them as tool-level errors. Returning (nil, err) here propagates the error as a server-level error, which may cause different client behavior (e.g., no structured error message).
Proposed fix for consistent error handling
err = request.BindArguments(&args)
if err != nil {
- return nil, err
+ return mcp.NewToolResultError(err.Error()), nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| err = request.BindArguments(&args) | |
| if err != nil { | |
| return nil, err | |
| } | |
| err = request.BindArguments(&args) | |
| if err != nil { | |
| return mcp.NewToolResultError(err.Error()), nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/mcp_impl/handlers.go` around lines 69 - 72, The error handling for
BindArguments is inconsistent with other error handling in this handler.
Currently, when BindArguments fails, the code returns (nil, err) which
propagates it as a server-level error. Refactor this to wrap the error in
mcp.NewToolResultError(...) and return (mcp.NewToolResultError(...), nil)
instead, matching the pattern used for other errors in the same handler such as
RequireString failures and interpreter errors. This ensures all tool-level
errors are handled consistently as structured tool result errors rather than
server-level errors.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/specs_format/index.go (1)
274-289:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
validateSpecsis defined but never called.The
validateSpecsfunction validates that balance lists don't contain duplicate entries, butCheck()never invokes it. Duplicate balance entries will silently use the last value instead of being rejected as the docstring intends.Add
if err := validateSpecs(specs); err != nil { return SpecsResult{}, ... }at the start ofCheck().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/specs_format/index.go` around lines 274 - 289, The validateSpecs function is defined but never invoked, allowing duplicate balance entries to silently use the last value instead of being rejected. Add a call to validateSpecs at the beginning of the Check() function to validate the specs Balances before any processing occurs; if validateSpecs returns an error, return a SpecsResult with the error rather than continuing.
♻️ Duplicate comments (2)
internal/interpreter/value.go (1)
44-49:⚠️ Potential issue | 🟠 Major | ⚡ Quick winScoped account strings passed as variable values are not parsed into separate Name/Scope.
When a caller passes an account string containing a scope suffix (e.g.,
"src/x"as a variable value),NewAccountAddressstores it entirely in theNamefield without extracting the scope. This meansAccountAddress{Name: "src/x", Scope: ""}is created instead of{Name: "src", Scope: "x"}.Balance queries and postings will then use the wrong keys, looking for
account="src/x", scope=""rather thanaccount="src", scope="x".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/interpreter/value.go` around lines 44 - 49, The NewAccountAddress function does not parse scoped account strings that contain a scope suffix. When a string like "src/x" is passed as the src parameter, the entire string is stored in the Name field without extracting the scope portion. Modify NewAccountAddress to parse the src string, extract any scope suffix (typically separated by a delimiter), and populate both the Name and Scope fields of the returned AccountAddress struct accordingly. The validation with checkAccountName should be applied appropriately to handle both scoped and unscoped account strings.internal/interpreter/interpreter.go (1)
199-206:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAccount regex rejects digit-containing scopes that
validateScopeaccepts.The regex pattern
(?:/[a-z_]+)?only allows lowercase letters and underscores in the scope suffix, butvalidateScopeaccepts digits (e.g.,"x1"). A script usingscoped(@src, "x1")will pass scope validation during interpretation but then failcheckPostingInvariantswhen the posting is created.Align the regex with
validateScopeby changing[a-z_]+to[a-z0-9_]+.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/interpreter/interpreter.go` around lines 199 - 206, The account name regex pattern in accountNameRegex uses a character class [a-z_]+ for the scope suffix that does not match the validation logic in validateScope, which accepts digits. Update the regex pattern to include digits by changing the scope suffix pattern from (?:/[a-z_]+)? to (?:/[a-z0-9_]+)? so that the regex accepts digit-containing scopes like "x1" that validateScope permits, preventing validation failures downstream.
🧹 Nitpick comments (3)
internal/interpreter/value_test.go (1)
43-51: ⚡ Quick winConsider adding test coverage for scoped address JSON marshaling.
The test only covers an
AccountAddresswithout a scope. Adding a test case forAccountAddress{Name: "abc", Scope: "xyz"}marshaling to"abc/xyz"would ensure the scope formatting logic is verified.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/interpreter/value_test.go` around lines 43 - 51, The TestMarshalAddress function currently only tests marshaling of AccountAddress with a Name field but no Scope. Add an additional test case within TestMarshalAddress to verify that AccountAddress with both Name and Scope fields (e.g., Name: "abc", Scope: "xyz") marshals correctly to the format "abc/xyz". This ensures the scope formatting logic is properly covered by the test suite.internal/interpreter/interpreter.go (1)
581-586: 💤 Low valueDirect map access on
CachedBalancesassumes account was pre-queried.The code accesses
s.CachedBalances[account]directly and returns an error if the key is missing. This relies on the balance query batching having populated that key. If the batching logic ever changes or misses a path, this will surface as an opaqueInvalidUnboundedAddressInScalingAddresserror rather than indicating the actual cache miss.This is a potential fragility but not an immediate bug given current batching coverage.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/interpreter/interpreter.go` around lines 581 - 586, The direct map access on s.CachedBalances[account] at the point where the ok check fails assumes the balance query batching has already populated that key, but provides no explicit verification or clear indication when a cache miss occurs. Instead of returning the generic InvalidUnboundedAddressInScalingAddress error, add logging or improve the error message to explicitly indicate that the account's balance was not found in the cache, making it clear that the issue is a cache miss rather than an unbounded address problem. This will help surface any gaps in the balance query batching logic more explicitly.internal/interpreter/value.go (1)
275-287: 💤 Low valueSilent fallback to scale=0 on parse failure may hide malformed assets.
When the scale suffix cannot be parsed as an integer,
GetBaseAndScalesilently returnsscale=0. This could mask malformed asset strings that look valid but have non-numeric scale suffixes (e.g.,"USD/abc").Consider whether callers need to distinguish between "no scale specified" and "invalid scale format".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/interpreter/value.go` around lines 275 - 287, The GetBaseAndScale function on the Asset type silently masks malformed asset strings by returning scale=0 when the scale suffix fails to parse as an integer, making it impossible for callers to distinguish between a missing scale and an invalid scale format. Modify the GetBaseAndScale function signature to return a third error value (changing from (string, int64) to (string, int64, error)), and when the strconv.ParseInt call fails, return the error instead of silently returning 0. This allows callers to detect and handle genuinely malformed assets like "USD/abc" rather than treating them the same as valid assets without a scale suffix.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/specs_format/index.go`:
- Around line 274-289: The validateSpecs function is defined but never invoked,
allowing duplicate balance entries to silently use the last value instead of
being rejected. Add a call to validateSpecs at the beginning of the Check()
function to validate the specs Balances before any processing occurs; if
validateSpecs returns an error, return a SpecsResult with the error rather than
continuing.
---
Duplicate comments:
In `@internal/interpreter/interpreter.go`:
- Around line 199-206: The account name regex pattern in accountNameRegex uses a
character class [a-z_]+ for the scope suffix that does not match the validation
logic in validateScope, which accepts digits. Update the regex pattern to
include digits by changing the scope suffix pattern from (?:/[a-z_]+)? to
(?:/[a-z0-9_]+)? so that the regex accepts digit-containing scopes like "x1"
that validateScope permits, preventing validation failures downstream.
In `@internal/interpreter/value.go`:
- Around line 44-49: The NewAccountAddress function does not parse scoped
account strings that contain a scope suffix. When a string like "src/x" is
passed as the src parameter, the entire string is stored in the Name field
without extracting the scope portion. Modify NewAccountAddress to parse the src
string, extract any scope suffix (typically separated by a delimiter), and
populate both the Name and Scope fields of the returned AccountAddress struct
accordingly. The validation with checkAccountName should be applied
appropriately to handle both scoped and unscoped account strings.
---
Nitpick comments:
In `@internal/interpreter/interpreter.go`:
- Around line 581-586: The direct map access on s.CachedBalances[account] at the
point where the ok check fails assumes the balance query batching has already
populated that key, but provides no explicit verification or clear indication
when a cache miss occurs. Instead of returning the generic
InvalidUnboundedAddressInScalingAddress error, add logging or improve the error
message to explicitly indicate that the account's balance was not found in the
cache, making it clear that the issue is a cache miss rather than an unbounded
address problem. This will help surface any gaps in the balance query batching
logic more explicitly.
In `@internal/interpreter/value_test.go`:
- Around line 43-51: The TestMarshalAddress function currently only tests
marshaling of AccountAddress with a Name field but no Scope. Add an additional
test case within TestMarshalAddress to verify that AccountAddress with both Name
and Scope fields (e.g., Name: "abc", Scope: "xyz") marshals correctly to the
format "abc/xyz". This ensures the scope formatting logic is properly covered by
the test suite.
In `@internal/interpreter/value.go`:
- Around line 275-287: The GetBaseAndScale function on the Asset type silently
masks malformed asset strings by returning scale=0 when the scale suffix fails
to parse as an integer, making it impossible for callers to distinguish between
a missing scale and an invalid scale format. Modify the GetBaseAndScale function
signature to return a third error value (changing from (string, int64) to
(string, int64, error)), and when the strconv.ParseInt call fails, return the
error instead of silently returning 0. This allows callers to detect and handle
genuinely malformed assets like "USD/abc" rather than treating them the same as
valid assets without a scale suffix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3b25e2ab-7af6-44c2-882c-bbc9c4753249
📒 Files selected for processing (18)
internal/interpreter/append_scope.gointernal/interpreter/append_scope_test.gointernal/interpreter/args_parser_test.gointernal/interpreter/balances.gointernal/interpreter/batch_balances_query.gointernal/interpreter/function_exprs.gointernal/interpreter/function_statements.gointernal/interpreter/funds_queue.gointernal/interpreter/funds_queue_test.gointernal/interpreter/get_involved_accounts.gointernal/interpreter/internal_balances.gointernal/interpreter/internal_balances_test.gointernal/interpreter/interpreter.gointernal/interpreter/interpreter_test.gointernal/interpreter/store.gointernal/interpreter/value.gointernal/interpreter/value_test.gointernal/specs_format/index.go
💤 Files with no reviewable changes (1)
- internal/interpreter/append_scope.go
✅ Files skipped from review due to trivial changes (1)
- internal/interpreter/internal_balances_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/interpreter/balances.go
- internal/interpreter/function_exprs.go
- internal/interpreter/args_parser_test.go
- internal/interpreter/interpreter_test.go
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #161 +/- ##
==========================================
+ Coverage 70.04% 70.81% +0.77%
==========================================
Files 55 58 +3
Lines 5024 5250 +226
==========================================
+ Hits 3519 3718 +199
- Misses 1313 1322 +9
- Partials 192 210 +18 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@numscript.go`:
- Line 63: The AccountMetadataRow alias on line 63 points to
interpreter.AccountMetadataRow which is not defined or exported, causing a build
failure. Either locate the correct exported type name from the interpreter
package and update the alias to reference that existing type, or ensure that
AccountMetadataRow is properly exported from the internal/interpreter package so
it can be aliased here.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 14d3feb5-8866-4448-9cf0-e92ffcf4d5b8
⛔ Files ignored due to path filters (6)
inputs.schema.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/metadata.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/neg-max-dest.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/override-account-meta.num.specs.jsonis excluded by!**/*.jsoninternal/interpreter/testdata/script-tests/set-account-meta.num.specs.jsonis excluded by!**/*.jsonspecs.schema.jsonis excluded by!**/*.json
📒 Files selected for processing (6)
internal/cmd/test_init.gointernal/interpreter/function_exprs.gointernal/interpreter/interpreter_test.gointernal/interpreter/store.gonumscript.gonumscript_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/interpreter/store.go
- internal/interpreter/interpreter_test.go
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/interpreter/accounts_metadata.go (1)
22-30: ⚡ Quick winInclude
Scope/Colorin metadata pretty-print output.Line 27 currently drops scoped/color dimensions, so distinct metadata rows can print identically and become hard to debug.
Suggested refactor
func (m AccountsMetadata) PrettyPrint() string { - header := []string{"Account", "Name", "Value"} + header := []string{"Account", "Name", "Value"} + showColor := false + showScope := false + for _, row := range m { + showColor = showColor || row.Color != "" + showScope = showScope || row.Scope != "" + } + if showColor { + header = append(header, "Color") + } + if showScope { + header = append(header, "Scope") + } var rows [][]string for _, row := range m { - rows = append(rows, []string{row.Account, row.Key, row.Value}) + out := []string{row.Account, row.Key, row.Value} + if showColor { + out = append(out, row.Color) + } + if showScope { + out = append(out, row.Scope) + } + rows = append(rows, out) } return utils.CsvPretty(header, rows, true) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/interpreter/accounts_metadata.go` around lines 22 - 30, The PrettyPrint method in AccountsMetadata is missing the Scope and Color dimensions in its output, which causes distinct metadata rows to print identically. Add "Scope" and "Color" to the header slice alongside "Account", "Name", and "Value", and then modify the loop that builds rows to include the Scope and Color fields from each row object (row.Scope and row.Color) when appending to the rows slice, ensuring all dimensions are captured in the formatted output.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/interpreter/accounts_metadata.go`:
- Around line 35-44: The CompareAccountsMetadata function uses slices.Contains
to check if elements from slice a exist in slice b, but this approach only
verifies presence and not count, allowing unequal datasets with duplicate values
to incorrectly return true. Fix this by implementing proper multiset comparison
that accounts for element counts: either sort both AccountsMetadata slices and
use slices.Equal for direct comparison, or use a map-based approach to count
occurrences of each element in both slices and verify the counts match. Ensure
the fix correctly handles the case where slices have equal length but different
element frequencies.
---
Nitpick comments:
In `@internal/interpreter/accounts_metadata.go`:
- Around line 22-30: The PrettyPrint method in AccountsMetadata is missing the
Scope and Color dimensions in its output, which causes distinct metadata rows to
print identically. Add "Scope" and "Color" to the header slice alongside
"Account", "Name", and "Value", and then modify the loop that builds rows to
include the Scope and Color fields from each row object (row.Scope and
row.Color) when appending to the rows slice, ensuring all dimensions are
captured in the formatted output.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: af3f1cfb-a4a2-4cfc-853a-d9e730c2b869
📒 Files selected for processing (6)
internal/interpreter/accounts_metadata.gointernal/interpreter/function_exprs.gointernal/interpreter/function_statements.gointernal/interpreter/internal_accounts_metadata.gointernal/interpreter/interpreter.gointernal/specs_format/index.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/interpreter/function_exprs.go
- internal/specs_format/index.go
- internal/interpreter/interpreter.go
ascandone
force-pushed
the
feat/scoped-accounts
branch
from
d353ea3 to
62c70e1
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
ascandone
force-pushed
the
feat/scoped-accounts
branch
from
62c70e1 to
c07d256
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
🛑 Changes requested — automated review
The runtime changes are mostly coherent, but the new slice-based metadata comparison can produce false-positive spec assertions when expected metadata contains duplicates.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
🛑 Changes requested — automated review
The runtime scoped-account path is mostly covered, but adding the builtin without updating the public involved-account analyzer leaves scoped scripts failing in that API.
|
@corerabbitai review |
Model accounts as a struct carrying an optional scope instead of encoding
it into the address string. Scope flows structured through balances,
postings, the funds queue, balance/metadata queries and the store.
Account metadata is now row-based externally (AccountsMetadata as a list of
AccountMetadataRow, mirroring Balances) and indexed by a {account, scope,
key} map internally (InternalAccountsMetadata) for fast lookups.
ascandone
force-pushed
the
feat/scoped-accounts
branch
from
f8c7fae to
4eb0731
Compare
🛑 Changes requested — automated reviewThis PR introduces scoped accounts to Numscript but has several confirmed issues that need resolution before merge. The most critical is a build-breaking undefined type alias in the public package. Several recurring themes across reviewers are: the wrong feature flag being used for Findings outside the diff🟠 [major] Account variable validation accepts @ prefix, which is never stripped — The new validation regex also accepts values like Suggestion: Either reject the 🟠 [major] Multi-statement BuildProgram produces invalid Numscript — When Suggestion: Insert a newline (or appropriate separator) between each rendered statement in the loop inside 🟠 [major] Removal of public involved-accounts package breaks existing consumers — Deleting the Suggestion: Keep a compatibility shim or reintroduce the API while updating its implementation to handle scoped accounts. |
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 10 new inline findings.
Summary: #161 (comment)
|
|
||
| AccountMetadata = interpreter.AccountMetadata | ||
| // Input account metadata (opaque, string-valued) read via meta() | ||
| AccountMetadataRow = interpreter.AccountMetadataRow |
There was a problem hiding this comment.
🔴 [blocker] Public type alias references undefined interpreter.AccountMetadataRow
reported by NumaryBot, coderabbitai
AccountMetadataRow is aliased to interpreter.AccountMetadataRow, but no such type is exported (or even exists) in internal/interpreter. This causes a build/typecheck failure confirmed by CI and multiple reviewers.
Suggestion: Either define and export AccountMetadataRow in internal/interpreter first, or point the public alias at the correct existing type before re-exporting it.
| VersionConstraints: []VersionClause{ | ||
| { | ||
| Version: parser.NewVersionInterpreter(0, 0, 25), | ||
| FeatureFlag: flags.ExperimentalScopedFunction, |
There was a problem hiding this comment.
🟠 [major] Wrong feature flag gates scoped() in static analysis
scoped() is currently gated by experimental-get-amount-function in the analyzer, while the interpreter checks ExperimentalScopedFunction. A script that enables the documented scoped flag still gets a diagnostic for the wrong flag; conversely, enabling only the get-amount flag can pass analysis but fail at runtime.
Suggestion: Change the constraint at this line to reference ExperimentalScopedFunction (matching the interpreter), not experimental-get-amount-function.
| return AccountAddress{}, InvalidAccountName{Name: src} | ||
| } | ||
| return AccountAddress(src), nil | ||
| return AccountAddress{Name: src}, nil |
There was a problem hiding this comment.
🟠 [major] Slash-scoped account strings accepted but not split into AccountAddress.Scope
When an account value arrives as a raw string such as src/x, the updated regex accepts it but NewAccountAddress stores the entire string in Name with an empty Scope. Balance and metadata queries therefore use account="src/x", scope="" instead of account="src", scope="x", so they cannot match scoped rows, and postings won't populate sourceScope/destinationScope.
Suggestion: Split the slash form inside NewAccountAddress: if the string contains /, set Name to the part before the slash and Scope to the part after.
| }, | ||
| }, | ||
| }, | ||
| FnVarOriginScoped: VarOriginFnCallResolution{ |
There was a problem hiding this comment.
🟠 [major] GetInvolvedAccounts has no case for scoped(), returning UnboundFunctionErr
Adding scoped() as a supported account expression was not mirrored in the involved-account discovery evaluator. Any script using scoped(@src, "x") in a source, destination, balance, or metadata position returns UnboundFunctionErr from ParseResult.GetInvolvedAccounts, breaking the public preflight API for the new feature.
Suggestion: Add a FnVarOriginScoped (or equivalent) case to get_involved_accounts.evalExpr that returns the scoped account address.
| Account string | ||
| Asset string | ||
| Color string | ||
| Scope string |
There was a problem hiding this comment.
🟠 [major] TestInitStore does not propagate Scope when seeding generated balances
When test-init funds an unknown scoped balance the returned BalanceRow has Scope set, but the balance is stored in StaticStore.Balances keyed and written without the scope. Generated specs for scoped scripts contain unscoped initial balances, so replaying the spec queries the scoped balance as zero and produces wrong expected balances/failures.
Suggestion: Include Scope in the key used to track funded balances and set BalanceRow.Scope when appending rows in TestInitStore.GetBalances.
| } | ||
|
|
||
| return utils.CsvPretty(header, rows, true) | ||
| return utils.CsvPrettyOmitEmptyCols(header, rows, true) |
There was a problem hiding this comment.
🟠 [major] CompareAccountsMetadata uses subset check instead of multiset comparison
slices.Contains only checks presence, not count. Two AccountsMetadata slices that differ only in duplicate rows can incorrectly compare as equal, masking spec assertion failures.
Suggestion: Use a frequency map: count occurrences of each row in slice a, then decrement for each row in b, returning false if any count goes below zero.
| @@ -73,7 +73,14 @@ func handleEvalTool(ctx context.Context, request mcp.CallToolRequest) (*mcp.Call | |||
| } | |||
There was a problem hiding this comment.
🟡 [minor] Inconsistent error handling for BindArguments failure
All other errors in this handler (e.g., RequireString, parse errors, interpreter errors) are returned as mcp.NewToolResultError(...) with a nil transport error. A BindArguments failure here propagates as a server-level error (nil, err), which may cause different client behavior.
Suggestion: Change return nil, err to return mcp.NewToolResultError(err.Error()), nil for consistency with the rest of the handler.
| if v.Scope == "" { | ||
| return fmt.Sprintf(`@%s`, v.Name) | ||
| } | ||
| return fmt.Sprintf(`scoped(%s, "%s")`, v.Name, v.Scope) |
There was a problem hiding this comment.
🟡 [minor] Scoped account Value.String() renders without the @ literal prefix
When a scoped account value is rendered via Value.String() (used for pretty output and spec comparisons), it produces scoped(alice, "reserve") instead of scoped(@alice, "reserve"). The DSL requires the @ prefix inside scoped(...), so any output copied for debugging or used in spec comparisons is not valid DSL.
Suggestion: Prepend @ to v.Name in the scoped account branch of Value.String().
| "required": ["type", "name"], | ||
| "properties": { | ||
| "type": { "const": "asset" }, | ||
| "name": { "type": "string", "pattern": "^([A-Z]+(/[0-9]+)?)$" } |
There was a problem hiding this comment.
🟡 [minor] Asset pattern in spec schema is narrower than the runtime's checkAssetName
The new Value schema for tagged metadata assets only allows letters plus an optional scale, but the interpreter's checkAssetName also accepts digits and an optional _SUFFIX (e.g., USD_TEST/2). Schema validation therefore rejects spec files that contain runtime-valid assets.
Suggestion: Align the asset regex in the schema with the pattern actually enforced by checkAssetName in the interpreter.
| Asset string `json:"asset"` | ||
| Amount *big.Int `json:"amount"` | ||
| Color string `json:"color,omitempty"` | ||
| Scope string `json:"scope,omitempty"` |
There was a problem hiding this comment.
🟡 [minor] Balances.PrettyPrint does not show the Scope column
Now that balances can differ by Scope, two rows differing only in scope appear identical in pretty-printed output, making debugging difficult.
Suggestion: Include a Scope column in PrettyPrint when at least one row has a non-empty scope.
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 9 new inline findings.
Summary: #161 (comment)
|
|
||
| AccountMetadata = interpreter.AccountMetadata | ||
| // Input account metadata (opaque, string-valued) read via meta() | ||
| AccountMetadataRow = interpreter.AccountMetadataRow |
There was a problem hiding this comment.
🔴 [blocker] Undefined public type alias causes build failure
reported by NumaryBot, coderabbitai
AccountMetadataRow is aliased to interpreter.AccountMetadataRow, but no such type exists in internal/interpreter. This causes a typecheck/build failure verified by CI (Go typecheck and build both error on this line). The public package is entirely unusable until this is resolved.
Suggestion: Define and export AccountMetadataRow in internal/interpreter (or rename it to match the existing metadata model), then reference it correctly in the alias.
| VersionConstraints: []VersionClause{ | ||
| { | ||
| Version: parser.NewVersionInterpreter(0, 0, 25), | ||
| FeatureFlag: flags.ExperimentalScopedFunction, |
There was a problem hiding this comment.
🟠 [major] scoped() is gated by the wrong feature flag in static analysis
The scoped() builtin is registered in the analyzer with the experimental-get-amount-function flag instead of experimental-scoped-function. This means: (1) scripts enabling the documented scoped flag still get a missing get-amount diagnostic from numscript check/LSP, and (2) scripts enabling only get-amount pass static analysis but fail at runtime because the interpreter checks ExperimentalScopedFunction. Raised consistently across many review passes.
Suggestion: Change the feature-flag constraint for the scoped builtin in the checker to reference experimental-scoped-function / ExperimentalScopedFunction, matching what the interpreter enforces at runtime.
| }, | ||
| }, | ||
| }, | ||
| FnVarOriginScoped: VarOriginFnCallResolution{ |
There was a problem hiding this comment.
🟠 [major] scoped() not handled in involved-account discovery (GetInvolvedAccounts)
Registering scoped as a valid account expression in the checker does not add a corresponding case in GetInvolvedAccounts (the involved-account evaluator's function switch). Any script using scoped(@src, "x") in a source, destination, balance, or meta expression returns UnboundFunctionErr from the public preflight API, breaking callers that use this API before execution.
Suggestion: Add a FnVarOriginScoped (or equivalent) case in GetInvolvedAccounts/evalExpr that resolves scoped account expressions to their underlying account address.
| return AccountAddress{}, InvalidAccountName{Name: src} | ||
| } | ||
| return AccountAddress(src), nil | ||
| return AccountAddress{Name: src}, nil |
There was a problem hiding this comment.
🟠 [major] Slash-scoped account strings not parsed into AccountAddress.Scope
When an account value is supplied as a variable/API value in the string form emitted by AccountAddress.String() (e.g., src/x), the new regex accepts it but NewAccountAddress stores the entire string in Name with an empty Scope. Balance and metadata lookups then query account="src/x", scope="" instead of account="src", scope="x", so they cannot match scoped balance rows or populate sourceScope/destinationScope in postings.
Suggestion: In NewAccountAddress, split the accepted string on / to populate Name and Scope separately when a slash is present.
| Account string | ||
| Asset string | ||
| Color string | ||
| Scope string |
There was a problem hiding this comment.
🟠 [major] Scope not preserved in test-init generated balance specs
TestInitStore.GetBalances keys and appends generated balances only by account/asset/color, omitting Scope. When test-init funds a scoped source (e.g., scoped(@src, "x")), the generated spec records an unscoped balance, so replaying the spec queries the scoped balance as zero and produces incorrect expected balances or failures.
Suggestion: Include Scope in the key used for deduplication and populate BalanceRow{Scope: b.Scope} when appending generated balance rows to the spec store.
| } | ||
|
|
||
| return utils.CsvPretty(header, rows, true) | ||
| return utils.CsvPrettyOmitEmptyCols(header, rows, true) |
There was a problem hiding this comment.
🟠 [major] CompareAccountsMetadata gives incorrect result for duplicate rows
CompareAccountsMetadata uses slices.Contains to check presence, which means it can return true for unequal multisets (e.g., [A, A] vs [A, B] when len matches). This can mask spec assertion failures when metadata rows are duplicated.
Suggestion: Use a frequency-count map: increment counts for each row in a, decrement for each row in b, and return false if any count goes below zero.
| @@ -73,7 +73,14 @@ func handleEvalTool(ctx context.Context, request mcp.CallToolRequest) (*mcp.Call | |||
| } | |||
There was a problem hiding this comment.
🟡 [minor] Inconsistent error handling for BindArguments failure
Other errors in the same handler return (mcp.NewToolResultError(...), nil) as tool-level errors, but a BindArguments failure returns (nil, err), propagating it as a server-level error. This may cause different client behavior (no structured error message).
Suggestion: Change the BindArguments error path to return mcp.NewToolResultError(err.Error()), nil to be consistent with other error returns in the handler.
| Asset string `json:"asset"` | ||
| Amount *big.Int `json:"amount"` | ||
| Color string `json:"color,omitempty"` | ||
| Scope string `json:"scope,omitempty"` |
There was a problem hiding this comment.
🟡 [minor] Scope not shown in balance pretty-print output
Balances.PrettyPrint renders only account/asset/color/amount, so two rows that differ only by scope are displayed as indistinguishable entries in pretty output.
Suggestion: Add a Scope column to the pretty-printer when any row in the result set has a non-empty scope.
| return CsvPretty(header, rows, sortRows) | ||
| } | ||
|
|
||
| keep := make([]bool, len(header)) |
There was a problem hiding this comment.
⚪ [nit] CsvPrettyOmitEmptyCols renders empty header for empty result sets
When rows is empty, every keep flag remains false, so the function renders a table with an empty header (|) for empty postings, balances, or metadata. The previous behavior showed expected column headers even for empty results.
Suggestion: Special-case an empty rows slice to keep all non-optional headers, or treat the header row itself as a signal to preserve column visibility.
2 participants
No description provided.